Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BC-7561 - separate job for failed batch deletion requests #5448

Merged
merged 46 commits into from
Feb 5, 2025

Conversation

virgilchiriac
Copy link
Contributor

@virgilchiriac virgilchiriac commented Jan 20, 2025

Description

Links to Tickets or other pull requests

BC-7561
hpi-schul-cloud/dof_app_deploy#1095

Approval for review

  • DEV: If api was changed - generate-client:server was executed in vue frontend and changes were tested and put in a PR with the same branch name.
  • QA: In addition to review, the code has been manually tested (if manual testing is possible)
  • All points were discussed with the ticket creator, support-team or product owner. The code upholds all quality guidelines from the PR-template.

this commit changes failed deletion and counted pending requests
 to also be older than configured threshold
add new config var for when pending or failed should
be no longer processed
@virgilchiriac virgilchiriac marked this pull request as ready for review January 20, 2025 10:49
@virgilchiriac virgilchiriac force-pushed the BC-7561-batch-delete-fix branch from 979f793 to ef53fad Compare January 20, 2025 13:06
@virgilchiriac virgilchiriac force-pushed the BC-7561-batch-delete-fix branch from a6bef9f to ee088b5 Compare January 20, 2025 18:37
@virgilchiriac virgilchiriac force-pushed the BC-7561-batch-delete-fix branch 2 times, most recently from ceaa2f4 to b9b6c83 Compare January 23, 2025 10:36
@virgilchiriac virgilchiriac force-pushed the BC-7561-batch-delete-fix branch from b9b6c83 to 8b8e624 Compare January 23, 2025 10:55
@virgilchiriac virgilchiriac force-pushed the BC-7561-batch-delete-fix branch from 165f606 to 51ea68e Compare January 23, 2025 13:01
@virgilchiriac virgilchiriac changed the title BC-7561 - batch delete fix BC-7561 - split batch delete failed requests Jan 23, 2025
apps/server/src/modules/user/service/user.service.spec.ts Outdated Show resolved Hide resolved
config/default.schema.json Outdated Show resolved Hide resolved
config/default.schema.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@@ -26,9 +26,9 @@ export class DeletionClient {
}
}

public async executeDeletions(limit?: number): Promise<void> {
public async executeDeletions(limit?: number, runFailed?: boolean): Promise<void> {
Copy link
Contributor

@uidp uidp Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to use an options object type instead of sereate parameters? Would better show the independence of limit and runFailed IMHO. (maybe also a matter of taste)

@virgilchiriac virgilchiriac requested a review from uidp January 27, 2025 15:59
@virgilchiriac virgilchiriac force-pushed the BC-7561-batch-delete-fix branch from 372ac41 to f0aaf59 Compare February 4, 2025 10:25
@virgilchiriac virgilchiriac force-pushed the BC-7561-batch-delete-fix branch from f0aaf59 to 4ebe5f8 Compare February 4, 2025 10:57
@virgilchiriac virgilchiriac enabled auto-merge (squash) February 5, 2025 15:00
};
const expectedQueryNoDates = {
status: {
$in: [StatusModel.FAILED, StatusModel.PENDING],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about filter for updatedAt: null ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have the case tested at line 126

@virgilchiriac virgilchiriac merged commit d8fc4e7 into main Feb 5, 2025
76 checks passed
@virgilchiriac virgilchiriac deleted the BC-7561-batch-delete-fix branch February 5, 2025 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants